Ensure release notes show most recent notes for pre-release version - #5770
Conversation
| // release notes (e.g. 2.11.4 -> 2.11.3 -> 2.11.2 -> ...). | ||
| final attemptedVersions = <String>[]; | ||
| var attempts = notesVersion.patch; | ||
| while (attempts >= 0 && notesVersion > versionFloor) { |
There was a problem hiding this comment.
If there are network issues requesting a release version we are looking for, it appears we might skip that and then try requesting the next version down.
Instead of searching for the version by looking for the first available high version, would it be possible for us to update our release process/helpers to change a constant in the project? It could signify the last release we have done, then we could just plug that constant in here, then try to fetch just that version.
It would probably have a healthier lifecycle too as we could choose how we want to fail if that specific request for the version doesn't end up succeeding.
There was a problem hiding this comment.
What release helper are you referring to? I worry that as we approach automated SDK releases, our build_release.sh script will be used to build dev releases and push them into the SDK, in addition to DevTools stable releases. But only stable releases will have release notes, so we wouldn't want build_release.sh to be updating some constant. It also might be difficult to enforce that a const in DevTools is updated when a change is made to the flutter website.
There was a problem hiding this comment.
If there are network issues requesting a release version we are looking for, it appears we might skip that and then try requesting the next version down
If the network requests all fail, we will eventually catch and log a warning with all the release notes versions we tried to fetch. This isn't new behavior from the previous state, though now we will try a max of 10 times where before we would only try 'patch' number of times where 'patch' was the current devtools version's patch value.
There was a problem hiding this comment.
hmm fair enough that we need to be in sync with the flutter website so that might be tricky. Although, In the current implementation if the flutter website wasn't updated yet, that we would still be showing the wrong release notes though :/ 🤔 We would technically show old release notes until the new ones were posted up.
In the case that the flutter website notes are up to date, I think the case I'd still be worried about is if only the request to the proper version fails and then the 2nd request passes. Then I think we would skip a version and show old release notes.
I think the current approach could be fine for now but I wonder if the better behaviour would be to show no release notes if the exact version we want to show isn't available.
I think if we wanted to let DevTools know which release notes to show, we could put a const here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/devtools.dart
We could update it any time we de a minor update in update_version.dart:
https://github.com/flutter/devtools/blob/master/tool/update_version.dart#L48
There was a problem hiding this comment.
Although, In the current implementation if the flutter website wasn't updated yet, that we would still be showing the wrong release notes though
Not necessarily. We only decrease patch versions looking for release notes. So for example, if I release DevTools 2.25.1 and I haven't published flutter website release notes for 2.25.1, then this code will only look for 2.25.1 and 2.25.0. We will not decrease the minor version and keep looking. We don't push up release notes for new patch versions - only for minor version bumps with the monthly release.
I think the case I'd still be worried about is if only the request to the proper version fails and then the 2nd request passes. Then I think we would skip a version and show old release notes.
The same scenario applies here. Because we only decrease patch versions looking for notes, and we only write release notes for minor versions, there is not a risk in the current state of showing release notes for the prior release.
What I do like about this approach is that release notes will just load if they are available, and we don't have another bit we have to set manually to ensure our system works. However, I see your point about adding a const so that we don't have to send more than one http request. Updating this const for every minor update does fall apart a little for bumping to a dev version after release though.
For example, when we released 2.23.1, we immediately follow that release with a version bump to 2.24.0-dev.1. So while 2.24.0-dev.1 is a minor + dev bump from 2.23.1, we still want to show the release notes for 2.23.1 if a user is on 2.24.0-dev.1
There was a problem hiding this comment.
SGTM about my concern with network stuff.
For the version bumping though, I might not have said it the right way. I guess it would be each time we make a release version we would be changing that const. Since those release versions are the ones that we are pushing up to the website.
For now though it seems that the searching method should be fine though.
2 participants
This will ensure that for a dev version like 2.24.0-dev.1, we will still show release notes for the most recent release (2.23.1 in this case). g3 users very rarely have a non-dev version of DevTools.